Release 0.7 mainnet fuzzing #783
Conversation
|
WalkthroughThe recent updates involve a series of changes across multiple smart contracts aimed at enhancing functionality, refining interactions with external data, and improving testing capabilities. These changes include updates to how price data is retrieved, adjustments to contract interactions based on specific conditions, shifts in function behavior from read-only to potentially state-modifying, modifications in contract setup procedures, and the introduction of new functions for testing purposes. Overall, these changes seek to enhance the system's flexibility, accuracy, and testing robustness. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
packages/contracts/package.jsonis excluded by:!**/*.json
Files selected for processing (2)
- packages/contracts/contracts/TestContracts/invariants/TargetContractSetup.sol (1 hunks)
- packages/contracts/contracts/TestContracts/invariants/echidna/EchidnaForkTester.sol (1 hunks)
Additional comments: 2
packages/contracts/contracts/TestContracts/invariants/echidna/EchidnaForkTester.sol (1)
- 13-15: The hardcoded values for
hevm.rollandhevm.warpare specific to a transaction and a timestamp, respectively. Ensure these values accurately represent the desired blockchain state for fuzz testing. It's also a good practice to document the rationale behind choosing these specific values for future reference and clarity.packages/contracts/contracts/TestContracts/invariants/TargetContractSetup.sol (1)
- 341-355: The updates to contract and token addresses in the
_setUpForkfunction are critical for aligning the testing environment with the current deployment setup. Ensure these new addresses are correct and point to the intended contracts. Additionally, consider documenting the source or rationale for each address change to maintain clarity and traceability. It's also important to verify that these changes do not introduce any security vulnerabilities and are consistent with the overall deployment strategy.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- packages/contracts/contracts/TestContracts/invariants/TargetContractSetup.sol (3 hunks)
- packages/contracts/contracts/TestContracts/invariants/TargetFunctions.sol (2 hunks)
- packages/contracts/contracts/TestContracts/invariants/echidna/EchidnaForkTester.sol (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/contracts/contracts/TestContracts/invariants/TargetContractSetup.sol
- packages/contracts/contracts/TestContracts/invariants/echidna/EchidnaForkTester.sol
Additional comments: 2
packages/contracts/contracts/TestContracts/invariants/TargetFunctions.sol (2)
- 1309-1309: The change to make the
setPricefunctionvirtualallows for overriding in derived contracts. This is a positive change for extensibility and flexibility in contract development. However, ensure that any overriding implementations maintain the integrity and security of price setting logic, especially considering the critical role of price information in the system.- 1323-1323: Making the
setGovernanceParametersfunctionvirtualenhances the contract's flexibility, allowing for customized governance logic in derived contracts. It's crucial to ensure that derived contracts implementing this function enforce proper access control and validation to maintain the system's integrity and prevent unauthorized or harmful changes to governance parameters.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/contracts/contracts/TestContracts/invariants/BeforeAfter.sol (4 hunks)
- packages/contracts/contracts/TestContracts/invariants/Properties.sol (8 hunks)
- packages/contracts/contracts/TestContracts/invariants/TargetFunctions.sol (10 hunks)
- packages/contracts/contracts/TestContracts/invariants/echidna/EchidnaForkTester.sol (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/contracts/contracts/TestContracts/invariants/TargetFunctions.sol
- packages/contracts/contracts/TestContracts/invariants/echidna/EchidnaForkTester.sol
Additional comments: 12
packages/contracts/contracts/TestContracts/invariants/BeforeAfter.sol (4)
- 115-115: The change from
collateral.getEthPerShare()tocollateral.getPooledEthByShares(1e18)reflects a shift towards a more precise method of calculating ETH per share, assuming1e18represents a standard precision for Ethereum tokens. This change seems logically sound, given the context of needing accurate calculations for financial contracts. However, ensure that the1e18constant is correctly aligned with the expected precision in your system, as hardcoding values can sometimes lead to errors if the underlying assumptions change.- 129-129: The conditional logic added around
crLens.quoteRealICR(_cdpId)ensures that the function is only called when_cdpIdis notbytes32(0). This is a good practice to avoid unnecessary calls or potential errors when dealing with null or default values. It's important to ensure that all possible_cdpIdvalues are considered and that this condition accurately captures the intended cases wherequoteRealICRshould not be called.- 172-172: Similar to the earlier comment, the use of
collateral.getPooledEthByShares(1e18)here is consistent with the change made in the_beforefunction. Consistency in using this method both before and after the operations is crucial for accurate comparisons and invariant testing. Again, verify that the hardcoded1e18precision is appropriate for your system's requirements.- 187-187: The conditional logic around
crLens.quoteRealICR(_cdpId)in the_afterfunction mirrors the change made in the_beforefunction, maintaining consistency in how these conditions are handled throughout the contract. This consistency is key in ensuring that the logic behaves as expected in all scenarios. The approval of this change is contingent on the earlier verification of the conditional logic's correctness and the assumption that_cdpIdvalues are appropriately handled.packages/contracts/contracts/TestContracts/invariants/Properties.sol (8)
- 165-167: The change from
getPrice()tofetchPrice()in theinvariant_SL_02function suggests a modification in how price data is retrieved. Ensure that thefetchPrice()method inPriceFeedTestnetis implemented correctly and provides the expected behavior, especially in terms of error handling and data freshness.- 185-188: Similar to the previous comment, the replacement of
getPrice()withfetchPrice()in theinvariant_SL_03function indicates a change in the price data retrieval mechanism. It's crucial to verify thatfetchPrice()behaves as intended, particularly regarding its reliability and accuracy in fetching the latest price data.- 242-252: In the
invariant_GENERAL_02function, the use offetchPrice()to obtain the current price for TCR calculations is noted. Given the critical nature of this calculation in ensuring the system's stability, thorough testing and validation of thefetchPrice()method's accuracy and error handling are essential.- 388-389: The introduction of
fetchPrice()in theinvariant_GENERAL_08function for TCR calculations from sums and system values highlights the importance of the accuracy and timeliness of price data. Confirm thatfetchPrice()provides reliable and up-to-date price information, as inaccuracies could impact the system's overall stability and security.- 441-441: The use of
fetchPrice()in theinvariant_GENERAL_12function to compare the real TCR quoted bycrLenswith the synced TCR fromcdpManagerunderscores the need for precise and current price data. Validate the implementation offetchPrice()to ensure it meets these requirements.- 453-453: Again, the replacement of
getPrice()withfetchPrice()in theinvariant_GENERAL_13function for ICR comparisons between real quotes and synced values emphasizes the critical role of accurate price data. Ensure thatfetchPrice()is implemented to provide precise and timely price information.- 507-507: The use of
fetchPrice()in theinvariant_LS_01function for liquidation sequencing comparisons between current and synced lists based on price highlights the necessity of accurate and up-to-date price data. Confirm thefetchPrice()method's reliability and accuracy in fetching the latest price data.- 530-531: The change to
fetchPrice()in theinvariant_DUMMY_01function, which checks for a positive price, points to the importance of ensuring thatfetchPrice()correctly handles errors and edge cases, such as returning zero or negative values.
aviggiano
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- packages/contracts/contracts/TestContracts/BaseStorageVariables.sol (1 hunks)
- packages/contracts/contracts/TestContracts/CRLens.sol (3 hunks)
- packages/contracts/contracts/TestContracts/invariants/Properties.sol (9 hunks)
- packages/contracts/contracts/TestContracts/invariants/TargetContractSetup.sol (4 hunks)
- packages/contracts/contracts/TestContracts/invariants/echidna/EchidnaForkTester.sol (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/contracts/contracts/TestContracts/invariants/Properties.sol
- packages/contracts/contracts/TestContracts/invariants/TargetContractSetup.sol
- packages/contracts/contracts/TestContracts/invariants/echidna/EchidnaForkTester.sol
Additional comments: 3
packages/contracts/contracts/TestContracts/BaseStorageVariables.sol (1)
- 24-24: The modification of the import statement for
CRLensto reference a local path is a good practice. It ensures that the contract uses the correct, project-specific version ofCRLens, which can help avoid potential issues with external dependencies changing or becoming incompatible.packages/contracts/contracts/TestContracts/CRLens.sol (2)
- 8-8: Adding the
IBorrowerOperationsinterface import is a necessary step for the contract to interact with borrower operations. This change aligns with the contract's expanded functionality.- 15-20: Modifying the constructor to include the
_borrowerOperationsaddress parameter and initializingborrowerOperationsis a crucial update for enabling the contract to interact with borrower operations. This change is correctly implemented and follows Solidity best practices for constructor arguments and immutability.
| @@ -42,6 +47,7 @@ contract CRLens { | |||
| /// @notice Return the ICR of a CDP after the fee split | |||
| /// @dev Call this from offChain with `eth_call` to avoid paying for gas | |||
| function getRealICR(bytes32 cdpId, bool revertValue) external returns (uint256) { | |||
| hevm.prank(address(borrowerOperations)); | |||
There was a problem hiding this comment.
The addition of hevm.prank(address(borrowerOperations)); in the getRealICR and getRealNICR functions is an interesting change. It's important to note that hevm.prank is typically used in testing environments to simulate transactions from specific addresses. Including it in production code could be a mistake unless this contract is specifically designed for testing purposes. If this is intended for a production environment, further clarification on the use of hevm.prank in these functions is needed to ensure security and correctness.
Consider removing or guarding hevm.prank calls if this contract is intended for production use. If it's for testing purposes, ensure that it's clearly documented and not deployable to production environments.
Also applies to: 69-69
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/contracts/foundry_test/BaseFixture.sol (2 hunks)
Additional comments: 2
packages/contracts/foundry_test/BaseFixture.sol (2)
- 30-30: The import path for
CRLenshas been updated. Ensure that the new path correctly points to theCRLenscontract within the project structure. This change is crucial for maintaining the integrity of imports and dependencies within the project.- 406-406: The instantiation of
CRLensnow includes an additional address parameter forborrowerOperations. This adjustment is necessary for theCRLenscontract to function correctly with the updated deployment setup. It's important to verify that theborrowerOperationsaddress passed here is correctly initialized and points to the intendedBorrowerOperationscontract instance. Additionally, ensure that any other parts of the codebase that interact withCRLensare aware of this change and adjust their interactions accordingly.Verification successful
The verification process has confirmed the presence and usage of the
borrowerOperationsaddress throughout the codebase in various contexts, including contract initializations, function calls, and logical checks. TheborrowerOperationsaddress is crucial for the correct functioning of the system, particularly in relation to theCRLenscontract instantiation, which now includes an additional address parameter forborrowerOperations. This adjustment is necessary for theCRLenscontract to function correctly with the updated deployment setup. The verification has also highlighted the importance of ensuring that any other parts of the codebase that interact withCRLensare aware of this change and adjust their interactions accordingly.Given the extensive references to
borrowerOperationsacross different files and the specific mention in the context of theCRLenscontract instantiation, it can be concluded that theborrowerOperationsaddress is correctly initialized and used throughout the codebase as needed. This supports the original review comment regarding the necessity of the adjustment for theCRLenscontract and the importance of verifying the correct initialization and usage of theborrowerOperationsaddress.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the borrowerOperations address is correctly initialized and used elsewhere as needed. rg --type solidity "borrowerOperations" --context 2Length of output: 84
Script:
#!/bin/bash # Adjusted script to verify that the borrowerOperations address is correctly initialized and used elsewhere as needed. # Searching for "borrowerOperations" in Solidity files without specifying an unrecognized file type. rg "borrowerOperations" --glob "*.sol" --context 2Length of output: 235043
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/contracts/foundry_test/EchidnaToFoundry.t.sol (1 hunks)
Additional comments: 2
packages/contracts/foundry_test/EchidnaToFoundry.t.sol (2)
- 28-31: The logic for determining the
senderand subsequently theactorin thesetupmodifier has been updated. This change dynamically assigns theactorbased on themsg.senderaddress, using a modulo operation to select betweenUSER1,USER2, andUSER3. Ensure that this logic correctly reflects the intended behavior and that the use of modulo operation does not introduce any unintended consequences or biases in how theactoris selected. Consider edge cases wheremsg.sendermight not fit the expected pattern or where the distribution betweenUSER1,USER2, andUSER3needs to be even.- 37-37: The
setUpfunction now explicitly assignsactortoactors[address(USER1)]. This change simplifies the assignment but assumes thatUSER1is always the desired actor for the initial setup. Verify that this assumption is valid across all test scenarios and that there's no need for dynamic actor selection in this context. If different actors are required for different tests, consider implementing a parameterized setup or additional logic to select the appropriate actor.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/contracts/foundry_test/CDPManager.redemptions.t.sol (2 hunks)
Additional comments: 1
packages/contracts/foundry_test/CDPManager.redemptions.t.sol (1)
- 492-492: The call to
vm.stopPrank()here is correctly placed if it's intended to end the simulation of transactions from theuseraddress. However, it's important to ensure that everyvm.startPrankhas a correspondingvm.stopPrankto avoid any potential issues with transaction context leakage between tests.
| @@ -476,7 +476,9 @@ contract CDPManagerRedemptionsTest is eBTCBaseInvariants { | |||
|
|
|||
| bytes32 _cdpId = _getFirstCdpWithIcrGteMcr(); | |||
|
|
|||
| vm.stopPrank(); | |||
There was a problem hiding this comment.
The call to vm.stopPrank() seems to be misplaced or unnecessary without a corresponding vm.startPrank(user) before it. This could lead to confusion or misinterpretation of the test's intention. It's important to ensure that vm.startPrank and vm.stopPrank are used in pairs to accurately simulate the actions of different users.
| _before(_cdpId); | ||
| vm.startPrank(user); |
There was a problem hiding this comment.
Similarly, the call to vm.startPrank(user) here is not preceded by a vm.stopPrank(), which might indicate an issue with the prank management in this test. It's crucial to manage the context of transactions accurately in tests to ensure they reflect real-world scenarios correctly.
| _after(_cdpId); | ||
| vm.startPrank(user); |
There was a problem hiding this comment.
Starting a prank without stopping it at the end of the test or before starting another prank could lead to unexpected behavior in subsequent tests. It's recommended to always pair vm.startPrank with vm.stopPrank within the same test to ensure isolation between tests.
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/contracts/foundry_test/EchidnaToFoundry.t.sol (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/contracts/foundry_test/EchidnaToFoundry.t.sol
Done
fetchPriceasgetPricedoes not exist on mainnetgetPooledEthBySharesasgetEthPerSharedoes not exist on mainnetCRLensreverting because ofsyncAccounting/syncGlobalAccountingInternalofAccruableCdpManager. On mainnet, these functions are blocked. Avm.prankis requiredSummary by CodeRabbit
New Features
IBorrowerOperationsinterface and updated constructor parameters inCRLens.sol.EchidnaForkTester.solfor future implementation.Bug Fixes
getEthPerShare()withgetPooledEthByShares(1e18)inBeforeAfter.sol.crLens.quoteRealICR(_cdpId)calls inBeforeAfter.sol.Refactor
viewkeyword toreturnsinProperties.sol.getPrice()withfetchPrice()across multiple contracts.Chores
TargetContractSetup.sol.BaseFixture.solandEchidnaToFoundry.t.sol.